Conversation
The monolithic `BillingKtx` class has been split into three distinct components to improve separation of concerns: - `BillingConnection`: Manages the connection to the `BillingClient` and observes purchase updates. - `BillingRepository`: Handles data-related operations like querying purchases, product details, and acknowledging/consuming products. - `BillingFlowLauncher`: Manages UI-related tasks such as launching the billing flow and showing in-app messages. The sample app has been updated to use these new components.
Summary of ChangesHello @oooRikOooo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the billing module by decomposing a large, monolithic class into smaller, more focused components. This change improves the architecture by adhering to the single-responsibility principle, making the codebase more maintainable, testable, and easier to understand. It also updates core dependencies to ensure compatibility and leverage the latest features and improvements. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of decomposing the monolithic BillingKtxImpl into smaller, single-responsibility classes: BillingConnection, BillingRepository, and BillingFlowLauncher. This significantly improves the library's design, making it more modular and maintainable. My review focuses on further enhancing this new design by programming to interfaces rather than concrete implementations. This will improve decoupling and make the components easier to test. I've also included a suggestion to improve consistency within the new classes. Overall, this is a great refactoring.
| private lateinit var connection: BillingConnectionImpl | ||
| private lateinit var repository: BillingRepositoryImpl | ||
| private lateinit var flowLauncher: BillingFlowLauncherImpl |
There was a problem hiding this comment.
It's a good practice to declare fields using interface types (BillingConnection, BillingRepository, BillingFlowLauncher) rather than concrete implementation types. This makes the code more flexible and decoupled from specific implementations. You'll need to add the corresponding imports for the interfaces.
| private lateinit var connection: BillingConnectionImpl | |
| private lateinit var repository: BillingRepositoryImpl | |
| private lateinit var flowLauncher: BillingFlowLauncherImpl | |
| private lateinit var connection: com.appsci.billingktx.client.connection.BillingConnection | |
| private lateinit var repository: com.appsci.billingktx.client.repository.BillingRepository | |
| private lateinit var flowLauncher: com.appsci.billingktx.client.ui.BillingFlowLauncher |
| fun connect(): Flow<BillingClient> | ||
|
|
||
| fun observeUpdates(): Flow<PurchasesUpdate> |
There was a problem hiding this comment.
To allow BillingRepository and BillingFlowLauncher to depend on this interface (as suggested in other comments), the withConnectedClient function should be part of the public contract of BillingConnection.
fun connect(): Flow<BillingClient>
fun observeUpdates(): Flow<PurchasesUpdate>
suspend fun <R> withConnectedClient(block: suspend (BillingClient) -> R): R| internal suspend fun <R> withConnectedClient( | ||
| block: suspend (BillingClient) -> R, | ||
| ): R { | ||
| return connectionFlow.map { | ||
| block(it) | ||
| }.first() | ||
| } |
There was a problem hiding this comment.
With withConnectedClient moved to the BillingConnection interface as suggested elsewhere, this implementation should be marked with override.
| internal suspend fun <R> withConnectedClient( | |
| block: suspend (BillingClient) -> R, | |
| ): R { | |
| return connectionFlow.map { | |
| block(it) | |
| }.first() | |
| } | |
| override suspend fun <R> withConnectedClient( | |
| block: suspend (BillingClient) -> R, | |
| ): R { | |
| return connectionFlow.map { | |
| block(it) | |
| }.first() | |
| } |
| internal fun isSuccess(@BillingClient.BillingResponseCode responseCode: Int): Boolean { | ||
| return responseCode == BillingClient.BillingResponseCode.OK | ||
| } |
There was a problem hiding this comment.
This utility function is used by other components (BillingRepositoryImpl, BillingFlowLauncherImpl). To avoid coupling those components to BillingConnectionImpl, it would be better to move this to a top-level internal function in a shared utility file (e.g., BillingUtils.kt). After moving it, you can remove it from this class.
| import kotlin.coroutines.resumeWithException | ||
|
|
||
| class BillingRepositoryImpl( | ||
| private val connection: BillingConnectionImpl, |
There was a problem hiding this comment.
To follow the Dependency Inversion Principle and improve testability, this class should depend on the BillingConnection interface instead of the BillingConnectionImpl implementation. This change will require exposing withConnectedClient on the BillingConnection interface and moving the isSuccess helper to a shared location. I've added separate comments with suggestions on how to achieve this.
| private val connection: BillingConnectionImpl, | |
| private val connection: com.appsci.billingktx.client.connection.BillingConnection, |
| override suspend fun isFeatureSupported(@FeatureType feature: String): Boolean { | ||
| return connection.withConnectedClient { | ||
| val result = it.isFeatureSupported(feature) | ||
| result.responseCode == BillingClient.BillingResponseCode.OK |
There was a problem hiding this comment.
For consistency, you should use a helper function to check for success here, just like in other methods of this class. Assuming isSuccess is moved to a top-level utility function as suggested in another comment, you can call it directly.
| result.responseCode == BillingClient.BillingResponseCode.OK | |
| isSuccess(result.responseCode) |
| import kotlin.coroutines.resumeWithException | ||
|
|
||
| class BillingFlowLauncherImpl( | ||
| private val connection: BillingConnectionImpl, |
There was a problem hiding this comment.
Similar to BillingRepositoryImpl, this class should depend on the BillingConnection interface for better decoupling and testability. This promotes loose coupling and makes the class easier to test in isolation.
| private val connection: BillingConnectionImpl, | |
| private val connection: com.appsci.billingktx.client.connection.BillingConnection, |
Introduced `BillingKtx` as a unified entry point to the library, simplifying its public API. This facade encapsulates the previously separate `BillingRepository`, `BillingFlowLauncher`, and `BillingConnection` components. Internal implementation details, including `BillingApi`, `BillingFlowLauncher`, and `BillingConnectionImpl`, are now marked as `internal` to hide them from consumers. Additionally, the Google Billing dependency is now exposed as an `api` dependency to allow consumers to use its types directly.
| import kotlin.coroutines.resumeWithException | ||
|
|
||
| internal class BillingFlowLauncher( | ||
| private val connection: BillingConnectionImpl, |
2 participants
Decompose monolithic
BillingKtxImpl(connection + queries + purchases + UI flows in one class) into single-responsibility components:BillingConnection/BillingConnectionImpl— connection lifecycle (connect(),observeUpdates(),withConnectedClient())BillingApi— data operations (getPurchases,getProductDetails,consumeProduct,acknowledge,getBillingConfig,isFeatureSupported)BillingFlowLauncher— Android UI operations that require Activity (launchFlow,showInappMessages)BillingKtxis now a concrete facade class (was an interface) that delegates to internal components and provides a simplified constructor — consumers no longer need to createBillingKtxFactorydirectly.Also bumps Gradle 8.11.1, Kotlin 2.2.0, AGP 8.9.1, and other dependencies.